fix: remove cpu core detection for preview semaphore #38385
Conversation
kesselb
force-pushed
the
fix-remove-auto-guessing-for-preview-semaphore
branch
from
f15d336
to
cb3861c
Compare
|
/backport to stable26 |
kesselb
requested review from
szaimen,
a team,
ArtificialOwl,
icewind1991 and
nfebe
and removed request for
a team
There was a problem hiding this comment.
This looks good but what if we maintain the checks on systems that we know how to?
I mean what if we add code that checks if the platform is one where we know how to detect the number of cores we just go ahead and do it?
I mean the feature would be beneficial for those on platforms which we support this feature on.
We can use something like PHP_OS_FAMILY see https://www.php.net/manual/en/reserved.constants.php
switch(PHP_OS_FAMILY) {
case 'Window':
//check ?
case 'BSD':
//check?
// More cases?
default:
//return the default preview counts without checking?
}
|
/backport to stable27 |
|
This would certainly address the issue for those impacted by it. But: what if we continue to try to determine as-is, but just make sure that a automated determination isn't a hard failure? (using the defaults when that happens). The other OSes/situations that are not currently functioning would then effectively function as this PR suggests. I realize this feature is configurable, but the automatic determination is pretty nice considering the wide array of hardware platforms NC is used on. Maintaining the automation (which works for a sizable % of the user base) goes a long way towards a positive experience / first impression in a a fairly visible area of the platform. @fenn-cs Unfortunately the checks would have to be more extensive than that because it's not just OS but also various local security restrictions. |
|
If anyone wishes to take over, please go ahead ;) The reason I'm removing it instead of extending it is because a configuration option is all you need to get the function going. Everything else, like finding out how many CPU cores there are, is optional. Please keep in mind that this optional part, to find out how many CPU cores are there, is executed every time a generator instance is requested. You can't even turn it off. I suggest setting up a setup check to let the admin know. If it is possible, we could even suggest a value. |
kesselb
force-pushed
the
fix-remove-auto-guessing-for-preview-semaphore
branch
from
cb3861c
to
772f932
Compare
kesselb
force-pushed
the
fix-remove-auto-guessing-for-preview-semaphore
branch
from
772f932
to
6c78f78
Compare
kesselb
force-pushed
the
fix-remove-auto-guessing-for-preview-semaphore
branch
from
6c78f78
to
939ef10
Compare
The idea of automatically limiting the number of concurrent preview requests to the number of CPU cores is nice. Unfortunately it's difficult to determinate the number of CPU cores (e.g. open_basedir, freebsd, etc.). This patch removes the detection and set the default for requests (existing + new) to 8 and generation to 4. Signed-off-by: Daniel Kesselberg <[email protected]>
kesselb
force-pushed
the
fix-remove-auto-guessing-for-preview-semaphore
branch
from
939ef10
to
30f8c07
Compare
kesselb
added
2. developing
Summary
The idea of automatically limiting the number of concurrent preview requests to the number of CPU cores is nice. Unfortunately it's difficult to determinate the number of CPU cores (e.g. open_basedir, freebsd, etc.).
This patch removes the detection and set the default for requests (existing + new) to 8 and generation to 4.
TODO
Checklist